feat: Make log_delivery_group input not-nullable #38
Conversation
This change is created to fix a problem when trying to use this module as DRY code.
The log_delivery_group variable expects a map, which it will turn into the log delivery configuration.
There is a default map provided in this module, which creates the slow-log log group.
1. Say you create a new root module calling this elasticache submodule. You want to use this same
root module for all your environments, and just pass the root module different values to determine
what it does in different environments.
2. You use this new root module to deploy production with the default values here. default slow-log group is created.
3. You deploy non-production, passing in a custom slow-log configuration. the custom slow-log is created.
4. You go back to deploy production, but there is a problem now. Since your root module is always
passing in some input variable to the elasticache submodule, the default value is always overridden.
The only way to get production to work again is to duplicate the default variable from this elasticache
module, into your own root module's variable, as its default.
Now you have to duplicate this module's variable defaults in your root module. Not very
DRY, and could cause issues later if the elasticache module changes and now your root module's defaults
don't apply anymore.
Add `nullable = false` to the elasticache module's input variable.
This prevents you from passing in a null value to the input.
If you pass in a null, Terraform will simply ignore the input as if it was never passed.
Terraform can't use a null here anyway; it dies saying it expects a map value, not a null.
So this doesn't break any existing functionality, but it does allow a root module to pass
a default `null` value for this input variable, which makes the elasticache module use
its default value.
pwillis-oi
changed the title
pwillis-oi
changed the title
pwillis-oi
changed the title
|
thats a lot to read 😅 and I didn't really follow it however, this is not a change we are looking to make at this time |
|
@bryantbiggs it's a one-line, backwards-compatible change that fixes your module so the user can force the module to use its defaults. please reconsider allowing the change. |
Incorrect - min supported Terraform version does not support
I don't see what is broken with the module. how its used, thats a different story, but as far as the module is concerned, there isn't anything thats been identified as broken |
Thank you for pointing that out, I thought it was supported in 1.0, apparently it was 1.1. As a workaround, I could submit a PR that sets the default to
The module is not broken (and I never claimed it was....?). There is functionality that the module does not support. This change "fixes" the module to support the functionality (it's pretty common functionality, to want to use a default or not, across submodule calls). This functionality is useful to users in general, as well as customers of AWS, who want to use this module in order to reduce the amount of maintenance they have to do. The point of these modules is to provide functionality to end users, so they can manage their infrastructure with less custom code and maintenance. This is one such piece of functionality that my company needs. Otherwise we have to copy parts of this module into a different module, just to work around this one specific problem. That adds a small but unnecessary maintenance burden. But it doesn't just affect me - it affects anyone else who has a similar use case. Hence why I opened this PR. I would add that, in the future, you might do well to be a little less dismissive and defensive to contributors. We're trying to help. If something is too long for you to read or understand, you can ask for a shorter clarification. If a fix doesn't seem backwards-compatible, you can ask me to find a different fix. You don't need to shut down the entire interaction before we've had time to have a conversation. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This change is created to fix a problem when trying to use this module as DRY code.
Description
Problem
The log_delivery_group variable expects a map, which it will turn into the log delivery configuration.
There is a default map provided in this module, which creates the slow-log log group.
Say you create a new root module calling this elasticache submodule. You want to use this same root module for all your environments, and just pass the root module different values to determine what it does in different environments.
You use this new root module to deploy production with the default values here. default slow-log group is created.
You want to deploy non-production with a custom slow-log. So you add an input variable when calling the
moduleblock, passing in a custom slow-log configuration. the custom slow-log is created.You go back to deploy production, but there is a problem now. Since your root module is always passing in some input variable to the elasticache submodule, the elasticache submodule's default value is always overridden. The only way to get production to work again is to duplicate the default variable from this elasticache module, into your own root module's variable, as its default.
Now you have to duplicate this module's variable defaults in your root module. Not very DRY, and could cause issues later if the elasticache module changes and now your root module's defaults don't apply anymore.
Solution
Add
nullable = falseto the elasticache module's input variable. This prevents you from passing in a null value to the input. If you pass in a null, Terraform will simply ignore the input as if it was never passed.Terraform can't use a null as the value anyway; it dies saying it expects a map value, not a null.
So this doesn't break any existing functionality, but it does allow a root module to pass a default
nullvalue for this input variable, which makes the elasticache module use its default value.Now you can use one DRY module, and decide to use the module's default variables or not, by passing null, or something else.
Motivation and Context
Getting a root module to pass input value sometimes, or pass
nullto use submodule's default value.Context is wanting to customize the log group for non-prod, but use only defaults for prod.
Breaking Changes
None
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request